Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch fix: add Manifest.toml to benchmarking #524

Merged
merged 11 commits into from
Nov 14, 2024

Conversation

SamuelBrand1
Copy link
Collaborator

After some problems with the benchmarking CI, I came to the conclusion that the failure was due to swapping to Pkg.add in CI which was causing uncommitted changes to the branch in the CI run leading to a failure ("dirty commit").

To stabilise the changes on the checked out branch in CI I've add a Manifest.toml file for the benchmarking following https://github.com/tkf/BenchmarkCI.jl#setup-with-benchmarkmanifesttoml .

  • The fork of BenchmarkCI we are using is specified there.
  • EpiAware dep is specified there.

This reduces the CI command to just instantiate and stops an error occurring between running benchmarks for the target and the baseline commits.

Evidence that this works is the test draft PR #523 which is aimed at benchmarking the test branch trigger-bci-2 against the test branch trigger-bci: both of which have this approach to specifying the benchmark project deps.

@SamuelBrand1 SamuelBrand1 requested a review from seabbs November 14, 2024 12:26
Copy link
Contributor

Try this Pull Request!

Open Julia and type:

import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/CDCgov/Rt-without-renewal", rev="patch-fix-benchmarkci-manifest", subdir="EpiAware")
using EpiAware

@seabbs seabbs enabled auto-merge November 14, 2024 12:43
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a nightmare and this all comes back to it not respecting source? It’s weird they show the add pattern working in their docs

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.64%. Comparing base (67d5072) to head (4683a6f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #524   +/-   ##
=======================================
  Coverage   89.64%   89.64%           
=======================================
  Files          53       53           
  Lines         753      753           
=======================================
  Hits          675      675           
  Misses         78       78           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seabbs seabbs added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit a5ff3b4 Nov 14, 2024
11 of 12 checks passed
@seabbs seabbs deleted the patch-fix-benchmarkci-manifest branch November 14, 2024 13:48
seabbs added a commit that referenced this pull request Dec 5, 2024
* Patch: Switch to fork of benchmarkCI (#520)

* patch to fork of benchmarkCI

* put fork version of BenchmarkCI in [sources]

* swap order

* add EpiAware [source]

* fix path

* rm benchmarkCI from project

* Patch fix: add `Manifest.toml` to benchmarking (#524)

* trigger

* Update benchmark.yaml

* Update benchmark.yaml

* commit benchmark Manifest

* try alternate approach

* Update benchmark.yaml

* Update EpiMethod.jl

* Update benchmark.yaml

* change baseline to origin/main

* remove trigger

* rm other trigger

* Issue 465: Add an infection generating model for ODE problems (#510)

* CompatHelper: bump compat for Turing to 0.35 for package EpiAware, (drop existing compat) (#516)

* CompatHelper: bump compat for Turing to 0.35 for package EpiAware, (drop existing compat)

* Update Project.toml

* fix Project.toml

---------

Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: Sam Abbott <[email protected]>
Co-authored-by: Samuel Brand <[email protected]>
Co-authored-by: Samuel Brand <[email protected]>

* CompatHelper: bump compat for DynamicPPL to 0.30 for package EpiAware, (drop existing compat) (#528)

Co-authored-by: CompatHelper Julia <[email protected]>

* rename IDD -> IID

* rename test file

* Issue 529: Create null Latent model (#530)

* Null Latent model

* Null Latent model

* fix doctest

* fix generate_epiaware unit tests

New usage of RW

* fix turing method test

underlying std of step size changed name

* fix broadcast test

Underlying std param changed name

* fix HN unit test

Default std prior had changed

* fix AR unit tests

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: Sam Abbott <[email protected]>
seabbs added a commit that referenced this pull request Dec 12, 2024
* draft MA method

* draft MA methd

* use IDD for epsilon in all models

* add MA benchmark

* Add docs and  tests for IDD

* make episilon_t a arg of the latent model constructors

* improve MA correctness

* fully import EpiAwareUtils

* add a test for IDD

* add tests for MA.jl

* add doc tests and unit tests + start on helper fn

* more updatres to AR appraoc

* chase down partial arg changes

* clean up AR

* clean up and add arma and arima helpers

* Contributions towards Arma/Arima models (#531)

* Patch: Switch to fork of benchmarkCI (#520)

* patch to fork of benchmarkCI

* put fork version of BenchmarkCI in [sources]

* swap order

* add EpiAware [source]

* fix path

* rm benchmarkCI from project

* Patch fix: add `Manifest.toml` to benchmarking (#524)

* trigger

* Update benchmark.yaml

* Update benchmark.yaml

* commit benchmark Manifest

* try alternate approach

* Update benchmark.yaml

* Update EpiMethod.jl

* Update benchmark.yaml

* change baseline to origin/main

* remove trigger

* rm other trigger

* Issue 465: Add an infection generating model for ODE problems (#510)

* CompatHelper: bump compat for Turing to 0.35 for package EpiAware, (drop existing compat) (#516)

* CompatHelper: bump compat for Turing to 0.35 for package EpiAware, (drop existing compat)

* Update Project.toml

* fix Project.toml

---------

Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: Sam Abbott <[email protected]>
Co-authored-by: Samuel Brand <[email protected]>
Co-authored-by: Samuel Brand <[email protected]>

* CompatHelper: bump compat for DynamicPPL to 0.30 for package EpiAware, (drop existing compat) (#528)

Co-authored-by: CompatHelper Julia <[email protected]>

* rename IDD -> IID

* rename test file

* Issue 529: Create null Latent model (#530)

* Null Latent model

* Null Latent model

* fix doctest

* fix generate_epiaware unit tests

New usage of RW

* fix turing method test

underlying std of step size changed name

* fix broadcast test

Underlying std param changed name

* fix HN unit test

Default std prior had changed

* fix AR unit tests

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: Sam Abbott <[email protected]>

* revert define_ namming

* clean out repeated utils from merge

* fix MA tests

* fix RW tests - feel made about RandomWalk vs AR naming

* fix remaining unit tests that aren't doctests

* update latent recovery test

* try and fix doctests automatically

* update all doctests to output nothing - this is awful

* add doctests for arima and arma

* fix doctest

* clean up deps

* update replication studies

* add interface tests for combination functions and add benchmarks

* add some basic theoretical properties tests

* name change IDD -> IID benchmarks

* moving all the constructors because this PR is too contained

* catch missing using

* update iid benchmark:

* update extraction

* remove old param namme from case study

* get the dot

* get the dot

* fix initial guess point for MAP opt

* Update index.jl

* add a compile time branch for HN

* add a compile time branch for HN

* update test

* add a new constructor to get old default behaviour

* update docs

* update docs - using the structs for priors is very brittle

* reorder prior plots

---------

Co-authored-by: Samuel Brand <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: Samuel Brand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants